Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Minor] Refactor parse Function #110

Closed
wants to merge 1 commit into from
Closed

[Minor] Refactor parse Function #110

wants to merge 1 commit into from

Conversation

el10savio
Copy link

Refactor parse function if all the characters in a base32 encoded ULID are part of the expected base32 character set into its own function.

Since the if condition checks if one of the entries is 0xFF and then returns true for ErrInvalidCharacters I refactored the break into a for loop that exits true early if the entry is 0xFF

@el10savio
Refactor Check if all the characters in a base32 encoded ULID are par…
bd95e00 
…t of the expected base32 character set into its own function
@peterbourgon
Copy link
Member

No objection, but I'll observe that the existing code will short-circuit in the if expr, so I think this change should be net-zero. But to be sure, please add a benchmark :)

@el10savio
Copy link
Author

el10savio commented Dec 28, 2023 •
edited
Loading

Oh yep. It looks like they're a bit different, added the benchstat output below.
It looks like now, with the refactor, it did get slower but processes less bytes/second

$~/go/ulid: ~/go/bin/benchstat old.txt new.txt 
goos: darwin
goarch: amd64
pkg: github.com/oklog/ulid/v2
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
              │   old.txt   │               new.txt               │
              │   sec/op    │   sec/op     vs base                │
ParseStrict-8   23.52n ± 1%   28.78n ± 1%  +22.36% (p=0.000 n=25)

              │    old.txt    │               new.txt                │
              │      B/s      │     B/s       vs base                │
ParseStrict-8   1054.2Mi ± 1%   861.6Mi ± 1%  -18.27% (p=0.000 n=25)

              │  old.txt   │            new.txt             │
              │    B/op    │    B/op     vs base            │
ParseStrict-8   0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=25) ¹
¹ all samples are equal

              │  old.txt   │            new.txt             │
              │ allocs/op  │ allocs/op   vs base            │
ParseStrict-8   0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=25) ¹
¹ all samples are equal
Benchmarking master ParseStrict 
$~/go/ulid: go test -count=25 -run='^$' -bench ^BenchmarkParseStrict$ -benchmem > old.txt 
goos: darwin
goarch: amd64
pkg: github.com/oklog/ulid/v2
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkParseStrict-8   	52033717	        22.28 ns/op	1167.14 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	56148057	        22.35 ns/op	1163.34 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	49870272	        24.63 ns/op	1055.82 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	50155957	        25.70 ns/op	1011.68 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51019125	        23.36 ns/op	1112.86 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	56249997	        23.35 ns/op	1113.26 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	49882028	        23.62 ns/op	1100.82 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51337762	        23.21 ns/op	1119.97 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	54401194	        22.90 ns/op	1135.24 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51512193	        23.08 ns/op	1126.36 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51861925	        22.88 ns/op	1136.47 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51102190	        23.45 ns/op	1108.55 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	54082071	        22.66 ns/op	1147.22 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51721083	        23.70 ns/op	1097.28 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51474328	        23.68 ns/op	1097.82 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	53599189	        23.61 ns/op	1101.14 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	50347875	        23.87 ns/op	1089.01 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	50605669	        23.67 ns/op	1098.47 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	50981748	        23.78 ns/op	1093.53 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	52730316	        23.52 ns/op	1105.43 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	50628523	        23.73 ns/op	1095.73 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	49521606	        23.93 ns/op	1086.68 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51417595	        23.51 ns/op	1105.89 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51914068	        23.78 ns/op	1093.36 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	51243764	        23.37 ns/op	1112.64 MB/s	       0 B/op	       0 allocs/op
PASS
ok  	github.com/oklog/ulid/v2	31.640s

Bechmarks after the refactor

$~/go/ulid: go test -count=25 -run='^$' -bench ^BenchmarkParseStrict$ -benchmem > new.txt
goos: darwin
goarch: amd64
pkg: github.com/oklog/ulid/v2
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkParseStrict-8   	42734313	        29.41 ns/op	 884.08 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	40853778	        27.37 ns/op	 949.94 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43455114	        26.58 ns/op	 978.06 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	42620434	        29.20 ns/op	 890.48 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	39307058	        29.16 ns/op	 891.49 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	42922989	        27.97 ns/op	 929.51 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	42980038	        30.28 ns/op	 858.77 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	39992593	        30.07 ns/op	 864.53 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	38569410	        29.63 ns/op	 877.39 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	39020055	        31.01 ns/op	 838.41 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43328752	        29.14 ns/op	 892.30 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	42873010	        28.27 ns/op	 919.68 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43154102	        29.27 ns/op	 888.24 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	40241872	        29.45 ns/op	 882.72 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	42981356	        28.67 ns/op	 906.90 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	45977173	        27.97 ns/op	 929.54 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	41245899	        29.15 ns/op	 892.02 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43414224	        28.74 ns/op	 904.52 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43423462	        27.47 ns/op	 946.51 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43386978	        27.61 ns/op	 941.58 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43705381	        28.75 ns/op	 904.46 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43167990	        27.76 ns/op	 936.67 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	43110888	        28.41 ns/op	 915.29 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	42695870	        28.72 ns/op	 905.19 MB/s	       0 B/op	       0 allocs/op
BenchmarkParseStrict-8   	41139072	        28.77 ns/op	 903.86 MB/s	       0 B/op	       0 allocs/op
PASS
ok  	github.com/oklog/ulid/v2	31.498s

@peterbourgon
Copy link
Member

Am I reading it correctly that this change is 22% more CPU and 18% less throughput i.e. strictly worse?

@el10savio
Copy link
Author

Yeah, looks like this refactor made it less performant.
Will close the PR to keep the current changes, thanks for the review @peterbourgon

@el10savio el10savio closed this Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@el10savio @peterbourgon